feat: option to force disable persisted operations#2007
Conversation
WalkthroughA new configuration option to disable persisted operations was introduced across the codebase. This includes adding a Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router/pkg/config/config.schema.json (1)
146-150: Align boolean‐field defaults withinpersisted_operations
disabledexplicitly sets"default": false, while neighbouring booleans such assafelist.enabledandlog_unknownomit a default (implicitlyfalse). Consider either:
- Dropping the redundant default here, or
- Adding
"default": falseto the other boolean siblingsto keep the schema style consistent and avoid signalling an unintended semantic difference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
router/core/graph_server.go(1 hunks)router/core/graphql_prehandler.go(4 hunks)router/core/operation_blocker.go(3 hunks)router/debug.config.yaml(1 hunks)router/pkg/config/config.go(2 hunks)router/pkg/config/config.schema.json(1 hunks)router/pkg/config/testdata/config_defaults.json(1 hunks)router/pkg/config/testdata/config_full.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
router/pkg/config/config.schema.json (1)
undefined
<retrieved_learning>
Learnt from: SkArchon
PR: #1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.376Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-router
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
router/debug.config.yaml (1)
42-43: LGTM! Minor formatting improvement.Adding a trailing newline follows POSIX text file standards and improves code formatting consistency.
router/pkg/config/testdata/config_defaults.json (1)
422-422: LGTM! Proper addition of the disabled flag.The new
Disabledfield correctly defaults tofalse, ensuring backwards compatibility while enabling the new feature to disable persisted operations.router/core/graph_server.go (1)
1209-1209: LGTM! Proper configuration propagation.The
PersistedOperationsDisabledfield correctly propagates the configuration setting frompersistedOperationsConfig.Disabledto theOperationBlockerOptions, enabling the operation blocker to respect the new disabled flag.router/pkg/config/testdata/config_full.json (1)
781-781: LGTM! Consistent test data update.The addition of the
Disabledfield maintains consistency with theconfig_defaults.jsonfile, ensuring comprehensive test coverage for the new persisted operations configuration option.router/pkg/config/config.go (3)
801-801: LGTM! New disabled field implementation is correct.The addition of the
Disabledfield with proper environment variable mapping and default value follows the established pattern in the codebase.
998-998: Good architectural improvement with envPrefix.The addition of
envPrefix:"PERSISTED_OPERATIONS_"provides a cleaner environment variable structure and explains the simplified tags in the nested struct fields.
802-803: Ensure backward-compatibility of renamed environment variablesWe’ve simplified the tags on
PersistedOperationsConfigso that:
LogUnknownnow only readsLOG_UNKNOWN(wasPERSISTED_OPERATIONS_LOG_UNKNOWN)Safelistnow uses its ownenvPrefix:"SAFELIST_"(wasPERSISTED_OPERATIONS_SAFELIST_)Without fallback tags or a migration guide, existing deployments that set
PERSISTED_OPERATIONS_LOG_UNKNOWNorPERSISTED_OPERATIONS_SAFELIST_*will no longer work. Please:• Confirm whether your config loader still recognizes the old
PERSISTED_OPERATIONS_*names or add explicit fallbacks (e.g.env:"PERSISTED_OPERATIONS_LOG_UNKNOWN;LOG_UNKNOWN")
• Update your documentation/CHANGELOG to list the old→new variable mappings and any required migration stepsLocations to review:
- router/pkg/config/config.go:802–803 (
LogUnknown,Safelist)- router/pkg/config/config.go:998 (the
envPrefix:"PERSISTED_OPERATIONS_"onPersistedOperationsConfig)router/core/operation_blocker.go (4)
6-6: Minor: Import reordering is acceptable.The "reflect" import was moved earlier in the import list, which doesn't affect functionality.
27-29: Excellent encapsulation improvement.Converting the exported fields to unexported (
SafelistEnabled→safelistEnabled,LogUnknownOperationsEnabled→logUnknownOperationsEnabled) and addingpersistedOperationsDisabledimproves the API design by preventing external modification of internal state.
56-56: LGTM! New option field follows established pattern.The
PersistedOperationsDisabledfield inOperationBlockerOptionsproperly supports the new functionality.
67-69: Constructor properly initializes new fields.The initialization of the new unexported fields from the options struct is correctly implemented and maintains consistency with the existing pattern.
router/core/graphql_prehandler.go (4)
482-482: LGTM! Consistent with encapsulation improvements.The field access update to use the unexported
safelistEnabledfield is consistent with the refactoring in operation_blocker.go.
489-489: Field access updates are correct.Both
safelistEnabledandlogUnknownOperationsEnabledfield accesses have been properly updated to use the new unexported fields.
542-542: Consistent field access pattern maintained.Another correct update to use the unexported field, maintaining consistency across the codebase.
584-584: Field access updates maintain consistency.The updates to use
logUnknownOperationsEnabledandsafelistEnabledare correctly implemented and consistent with the encapsulation improvements.Also applies to: 586-586
endigma
force-pushed
the
jesse/eng-7462-option-to-disable-po
branch
from
f74e95f to
52fb144
Compare
endigma
force-pushed
the
jesse/eng-7462-option-to-disable-po
branch
from
52fb144 to
4b8601d
Compare
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Checklist